Skip to content

feat: integrate mailing list service (CM-1318)#4346

Open
themarolt wants to merge 44 commits into
mainfrom
feat/mailing-list-integration-CM-1318
Open

feat: integrate mailing list service (CM-1318)#4346
themarolt wants to merge 44 commits into
mainfrom
feat/mailing-list-integration-CM-1318

Conversation

@themarolt

Copy link
Copy Markdown
Contributor

Summary

Integrate mailing list (public-inbox) support into CDP as a containerized worker. This implements email ingestion from public-inbox repositories (like LKML), mirroring shards, parsing emails, and emitting activities to Kafka for the data sink.

  • New mailing_list_integration service (Python, FastAPI)
  • Database schema for list management and processing state
  • Public-inbox integration via subprocess (mirrors, incremental fetch)
  • Email parser ported from noteren
  • Kafka producer for activity ingestion
  • Docker image for containerized deployment

How it works

  1. Worker polls mailinglist.listProcessing with FOR UPDATE SKIP LOCKED
  2. For each list, calls public-inbox-clone (initial) or public-inbox-fetch (incremental)
  3. Discovers shards (0.git, 1.git, etc.) and walks new commits since last processed
  4. Parses each email blob, emits groupsio-platform activity JSON
  5. Inserts results into integration.results (state=pending)
  6. Sends Kafka message to data-sink-worker for activity processing
  7. Updates lastProcessedHeads (per-shard tracking) and marks list as COMPLETED

Test coverage

  • 54 pytest tests (email parser) remain green
  • Ruff lint clean on all new Python files
  • Database schema migrates cleanly
  • Server imports and FastAPI lifespan tested

Out of scope

  • Onboarding UI to create integrations + mailinglist.lists rows
  • Member join/leave derivation (message ingestion only initially)

This PR targets hardcoded integrations for initial deployment; UI follows in a separate ticket.

themarolt added 11 commits July 14, 2026 18:50
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings July 15, 2026 10:56
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Large new ingestion path (SSRF-mitigated URLs, subprocess mirroring, Kafka/DB state) plus member-identity handling changes in the shared activity sink that affect all platforms’ batch creates.

Overview
Adds end-to-end public-inbox / lore mailing list ingestion: a new Python mailing_list_integration worker mirrors lists via public-inbox-clone/fetch, parses commits into message activities, writes integration.results, and emits Kafka for data-sink-worker.

Backend & data: PUT /mailing-list-connect (validated name + https sourceUrl) and IntegrationService.mailingListConnectOrUpdate upsert integrations and mailinglist.* rows; migrations add the mailinglist schema and activity type. Dev scripts onboard a default tenant and create integrations from JSON until UI ships.

Ops: Docker/compose/CLI wiring for the worker; CI runs ruff and pytest on the new app (git_integration path trigger also treats pyproject.toml/uv.lock as Python changes).

Ingestion fixes: data_sink_worker lowercases member keys for dedup and handles an extra verified-identity unique constraint by attaching to the existing member on create retries. Parser ADR/doc: skip messages with implausible Date years so bad timestamps never break the pipeline.

Reviewed by Cursor Bugbot for commit e46243a. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a containerized Python worker for ingesting public-inbox mailing lists into CDP through Kafka and integration.results.

Changes:

  • Adds mirroring, email parsing, processing, and Kafka publishing.
  • Adds mailing-list database schema and development seed.
  • Adds container, Compose, CI, and local tooling.

Review note: This exceeds the recommended 1,000-line PR target.

Reviewed changes

Copilot reviewed 26 out of 35 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
services/apps/mailing_list_integration/src/test/test_noteren.py Adds parser and CLI tests.
services/apps/mailing_list_integration/src/runner.sh Starts Uvicorn.
services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Implements ingestion loop.
services/apps/mailing_list_integration/src/crowdmail/worker/__init__.py Initializes worker package.
services/apps/mailing_list_integration/src/crowdmail/settings.py Defines environment configuration.
services/apps/mailing_list_integration/src/crowdmail/services/queue/queue_service.py Adds Kafka producer.
services/apps/mailing_list_integration/src/crowdmail/services/queue/__init__.py Initializes queue package.
services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py Parses public-inbox emails.
services/apps/mailing_list_integration/src/crowdmail/services/parse/__init__.py Initializes parser package.
services/apps/mailing_list_integration/src/crowdmail/services/mirror/mirror_service.py Manages public-inbox mirrors.
services/apps/mailing_list_integration/src/crowdmail/services/mirror/__init__.py Initializes mirror package.
services/apps/mailing_list_integration/src/crowdmail/services/__init__.py Initializes services package.
services/apps/mailing_list_integration/src/crowdmail/server.py Adds FastAPI lifecycle and health route.
services/apps/mailing_list_integration/src/crowdmail/models/list.py Defines mailing-list model.
services/apps/mailing_list_integration/src/crowdmail/models/__init__.py Initializes models package.
services/apps/mailing_list_integration/src/crowdmail/logger.py Configures Loguru.
services/apps/mailing_list_integration/src/crowdmail/errors.py Defines service errors.
services/apps/mailing_list_integration/src/crowdmail/enums.py Defines service enums.
services/apps/mailing_list_integration/src/crowdmail/database/registry.py Adds database helpers.
services/apps/mailing_list_integration/src/crowdmail/database/crud.py Adds processing-state queries.
services/apps/mailing_list_integration/src/crowdmail/database/connection.py Manages asyncpg pooling.
services/apps/mailing_list_integration/src/crowdmail/database/__init__.py Initializes database package.
services/apps/mailing_list_integration/src/crowdmail/__init__.py Initializes application package.
services/apps/mailing_list_integration/README.md Documents operation and setup.
services/apps/mailing_list_integration/pyproject.toml Defines package and tooling.
services/apps/mailing_list_integration/Makefile Adds development commands.
services/apps/mailing_list_integration/dev/seed.sql Seeds an example integration.
scripts/services/mailing-list-integration.yaml Adds Compose services.
scripts/services/docker/Dockerfile.mailing_list_integration Builds the worker image.
scripts/cli Registers the service in clean-start handling.
scripts/builders/mailing-list-integration.env Configures image publishing.
backend/src/database/migrations/V1784048135__mailinglist-schema.sql Creates mailing-list tables.
.github/workflows/backend-lint.yaml Adds Python linting CI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/services/docker/Dockerfile.mailing_list_integration
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread services/apps/mailing_list_integration/pyproject.toml
Comment thread services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py Outdated
Comment thread .github/workflows/backend-lint.yaml
Signed-off-by: Uroš Marolt <uros@marolt.me>
…ion (CM-1318)

Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
…318)

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings July 20, 2026 09:17
Comment thread services/apps/mailing_list_integration/src/crowdmail/server.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 51 changed files in this pull request and generated 16 comments.

Comments suppressed due to low confidence (3)

services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:90

  • Initial onboarding materializes every commit ID and retains every parsed DB/Kafka record until the entire archive finishes. For LKML-scale archives this is unbounded memory usage and no progress is checkpointed before a likely OOM/restart. Process bounded commit batches and persist each batch/head checkpoint before continuing, similar to the 250-commit chunking in git_integration/src/crowdgit/services/commit/commit_service.py:709-743.
    .github/workflows/backend-lint.yaml:147
  • This CI job installs pytest and the PR adds a substantial test suite, but the job runs only Ruff. As a result, parser regressions can merge even though the PR reports the tests as coverage. Run pytest in this job after lint/format checks.
      - name: Check Python linting and formatting
        if: steps.changes.outputs.python_changed == 'true'
        run: |
          uv run ruff check src/ --output-format=github
          uv run ruff format --check src/

services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:31

  • This introduces a new class-based worker, contrary to the repository's functions-over-classes direction for new service code (CLAUDE.md:42-43 and services-checklist.md:41-43). Keep shutdown state in a small functional runner/context and expose plain processing functions so the polling and per-list logic remain independently testable.

Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread backend/src/api/integration/helpers/mailingListAuthenticate.ts Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py Outdated
Comment thread services/apps/mailing_list_integration/README.md Outdated
Comment thread services/apps/mailing_list_integration/README.md Outdated
Comment thread services/apps/mailing_list_integration/README.md Outdated
Comment thread services/apps/mailing_list_integration/README.md Outdated
Comment thread services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py Outdated
Copilot AI review requested due to automatic review settings July 20, 2026 16:49
Comment thread services/apps/mailing_list_integration/uv.lock
Comment thread .github/workflows/backend-lint.yaml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 51 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (10)

services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:84

  • This ignores the stored source_url; ensure_mirror always clones https://lore.kernel.org/{name}/. A valid public-inbox URL with a different host or path will silently ingest the wrong archive. Pass the source URL to the mirror layer and keep the local directory key separate.
    services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:91
  • On initial onboarding, new_commits(..., None) materializes the shard’s entire history, and this method then retains every DB and Kafka record until all shards finish. Public-inbox shards can contain hundreds of thousands of messages, so the worker can exhaust memory before reaching the existing insert chunks. Iterate in bounded batches and checkpoint each batch only after persistence and emission succeed.
    services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:93
  • read_email launches synchronous Git subprocesses on the FastAPI event loop—twice per message. A large import will block health requests, shutdown handling, and other async work for long periods. Offload this blocking call to a thread.
    services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:94
  • Any parser exception escapes this per-commit loop, marks the whole list failed, and leaves the shard head unchanged. The next retry starts from the same malformed message, so one poison email can permanently block every later message in the shard. Catch failures per commit and skip/quarantine them with observable logging or metrics, as the Git integration does in commit_service.py:652-655.
    services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py:72
  • This helper is called by the long-running worker, so sys.exit() raises SystemExit, bypasses its except Exception handlers, and can terminate the service because of one malformed commit. Raise a domain exception and let the worker apply its per-message failure policy instead.
    services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py:87
  • The blob-read failure also calls sys.exit(). In worker mode this can terminate the entire service instead of failing one message/list. Raise a normal domain exception here, consistent with the commit-not-found branch.
    services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py:354
  • Messages without Message-ID are emitted with an empty sourceId. Activity deduplication keys include sourceId, so multiple such messages in the same segment/platform/type/channel can collapse into one activity. Use a stable fallback such as the Git commit/blob ID, or explicitly skip these messages.
    services/apps/mailing_list_integration/src/crowdmail/services/mirror/mirror_service.py:58
  • The five-minute default applies to initial public-inbox-clone and fetch operations. LKML-scale archives can legitimately take much longer, so onboarding will repeatedly kill the clone and never complete. Use a separately configurable mirror timeout sized for large archives while retaining shorter limits for lightweight commands.
    services/apps/mailing_list_integration/src/crowdmail/database/crud.py:117
  • A process crash after acquisition leaves state='processing' and lockedAt set. This exclusion, combined with both selectors requiring lockedAt IS NULL, means the list is never acquired again; stale onboarding rows also consume the concurrency count forever. Treat locks as leases and reclaim/reset processing rows older than a configured threshold.
    states_to_exclude = (ListState.PENDING, ListState.PROCESSING, ListState.PENDING_REONBOARD)

services/apps/mailing_list_integration/README.md:16

  • This documentation still says the new worker emits platform=groupsio, while the implementation, seed, activity type, and identities all use platform=mailinglist. The same stale Groups.io references recur at lines 46, 62, and 66; update them so operators verify the actual platform.
  parses new messages, writes activities to `integration.results`, and emits
  Kafka messages to the `data-sink-worker` topic — same plumbing as
  git_integration, with `platform=groupsio`.

Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py Outdated
Comment thread backend/src/database/migrations/V1784291394__add_mailinglist_activity_type.sql Outdated
Copilot AI review requested due to automatic review settings July 22, 2026 10:02

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 5 total unresolved issues (including 4 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d698cc9. Configure here.

"updatedAt" = NOW()
WHERE "listId" = $2
"""
await execute(sql_query, (heads, list_id))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head checkpoint lacks retry

Medium Severity

After each flush the worker writes DB rows, publishes to Kafka, then calls update_processed_heads with no retry. mark_list_processed is retried on failure, but a transient error on the head update fails the run after messages were already queued, so the next attempt re-reads the same commits and emits duplicate Kafka work for the same resultIds.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d698cc9. Configure here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

services/libs/data-access-layer/src/mailinglist/index.ts:76

  • Re-onboarding a previously removed URL hits this conflict branch, but the update leaves deletedAt populated. The worker explicitly filters deleted lists, so the connect call succeeds while the list is never processed. Also reset listProcessing when ownership moves to a different integration; otherwise the new project inherits the old shard heads and misses the archive.
    services/libs/data-access-layer/src/mailinglist/index.ts:72
  • The preceding ownership query does not make this upsert atomic. Two integrations connecting the same new sourceUrl can both observe no conflict; after one inserts, the other executes this DO UPDATE and silently steals the list. Enforce active ownership in the conflict statement itself and translate a rejected update into the existing 400 response.
    backend/src/api/integration/helpers/mailingListAuthenticate.ts:28
  • sourceUrl is later passed to public-inbox-clone, so accepting any non-empty string allows an editor to make the worker access local paths or arbitrary network targets. Restrict this to supported public HTTP(S) URLs and reject loopback/private destinations (including redirects), with defense-in-depth validation in the worker.
        sourceUrl: z.string().trim().min(1),

services/apps/mailing_list_integration/src/crowdmail/database/crud.py:83

  • This count check does not serialize concurrent acquisitions. For example, when two onboardings are active and the limit is three, two workers can both read count = 2, lock different pending rows via SKIP LOCKED, and raise the active count to four. Use a transaction-scoped advisory lock or a locked semaphore row around the count-and-acquire operation.
            AND c.count < $3

Comment on lines +1445 to +1447
if (lists.length === 0) {
this.options.log.warn('No lists provided - skipping mailing list integration update')
return null
Copilot AI review requested due to automatic review settings July 22, 2026 10:10
Signed-off-by: Uroš Marolt <uros@marolt.me>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (6)

backend/src/api/integration/helpers/mailingListAuthenticate.ts:28

  • sourceUrl is accepted as any non-empty string and later passed to public-inbox-clone from the worker. A tenant editor can therefore make the service access local schemes or internal network endpoints. Restrict this to supported public-inbox URL schemes and enforce a public-host/private-address policy before cloning.
    return false

backend/src/api/integration/helpers/mailingListAuthenticate.ts:23

  • Reject duplicate sourceUrl entries at validation time. PostgreSQL raises ON CONFLICT DO UPDATE command cannot affect row a second time when the same URL appears twice in this array, turning validly shaped client input into a 500 response.
// bare non-URL string.

services/libs/data-access-layer/src/mailinglist/index.ts:76

  • Re-onboarding a previously removed URL leaves deletedAt populated because the conflict update never clears it. The worker filters deleted rows, so the API reports success but the list is never processed again.
    services/libs/data-access-layer/src/mailinglist/index.ts:83
  • When a soft-deleted list is assigned to a different integration/segment, this preserves the previous owner's lastProcessedHeads. The new project then starts after those heads and permanently misses the archived messages that were ingested only into the old segment. Reset processing state and heads when ownership changes, while preserving them for same-integration updates.
    services/apps/mailing_list_integration/src/crowdmail/database/crud.py:83
  • The onboarding cap is not atomic. Concurrent workers can read the same c.count < MAX_CONCURRENT_ONBOARDINGS snapshot, lock different pending rows via SKIP LOCKED, and both exceed the configured limit. Serialize the count-and-acquire decision (for example with a transaction advisory lock or a dedicated semaphore row).
            AND c.count < $3

services/apps/mailing_list_integration/src/crowdmail/server.py:13

  • The PR description says the FastAPI lifespan is tested, but the service's only test module is test_noteren.py and contains no lifespan, worker, database, mirror, or queue tests. Add coverage for startup/shutdown and worker-task cancellation, or correct the stated test coverage.

`
SELECT "sourceUrl"
FROM mailinglist.lists
WHERE "integrationId" != $(integrationId)::uuid
Copilot AI review requested due to automatic review settings July 22, 2026 10:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

services/libs/data-access-layer/src/mailinglist/index.ts:76

  • Re-onboarding a previously removed source cannot work correctly here. The conflict check intentionally ignores soft-deleted rows, but this conflict branch neither clears deletedAt nor resets listProcessing; the worker therefore filters the reclaimed row out, and simply clearing deletedAt would still preserve the previous project's shard heads and skip its archive. Resurrect the row and conditionally reset processing state when ownership/segment changes, while preserving progress only for the same integration.
    backend/src/api/integration/helpers/mailingListAuthenticate.ts:27
  • Checking only the https: scheme still lets any tenant editor make the worker connect to arbitrary internal hosts or private IPs via public-inbox-clone (including through redirects), creating an SSRF path. Restrict sources to an approved public-inbox host allowlist, or resolve and reject loopback/private/link-local destinations and enforce the same policy for every redirect.
const isSafeSourceUrl = (sourceUrl: string): boolean => {
  try {
    return new URL(sourceUrl).protocol === 'https:'
  } catch {

backend/src/api/integration/helpers/mailingListAuthenticate.ts:47

  • Duplicate detection compares raw strings, so equivalent archive URLs such as https://lore.kernel.org/list and https://LORE.KERNEL.ORG/list/ bypass both this check and the database uniqueness/conflict guard. Normalize the URL before validation and persistence (hostname casing, default port, trailing slash, and disallow query/fragment/userinfo) so one archive has one canonical key.
    .refine((lists) => new Set(lists.map((l) => l.sourceUrl)).size === lists.length, {
      message: 'lists contains duplicate sourceUrl entries',
    }),

.github/workflows/backend-lint.yaml:123

  • Dependency-only changes to pyproject.toml or uv.lock make this job report success without installing dependencies or running tests because this condition only matches .py files. Include those project files in the change predicate so a broken lockfile or dependency update cannot bypass CI.
          if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q "^services/apps/mailing_list_integration/.*\.py$"; then

services/apps/mailing_list_integration/pyproject.toml:10

  • The package metadata points to the repository's Apache-2.0 LICENSE, but the imported noteren.py module declares GPL-2.0-or-later, so the built/distributed crowdmail package has conflicting license declarations. Resolve the licensing for the combined package (or isolate the GPL component) and make the package metadata/notices match the resulting distribution terms.
license = { file = "LICENSE" }

services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:160

  • The new ingestion core has no tests for batching/checkpoint order or DB/Kafka failure recovery; the only test module exercises the parser. Add worker tests proving heads advance only after a successful Kafka send, skipped messages still advance, and failed sends remain retryable without duplicate result rows.

Copilot AI review requested due to automatic review settings July 22, 2026 10:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (4)

services/apps/mailing_list_integration/src/crowdmail/database/crud.py:86

  • The onboarding limit is not atomic. Multiple workers can read the same current_onboarding_count, lock different pending rows with SKIP LOCKED, and all pass c.count < $3, exceeding MAX_CONCURRENT_ONBOARDINGS. Serialize the count-and-acquire decision (for example with an advisory/sentinel lock) or use a permit model so concurrent clone load is actually bounded.
            AND l."deletedAt" IS NULL
            AND c.count < $3
        ORDER BY lp.priority ASC, lp."createdAt" ASC
        LIMIT 1
        FOR UPDATE OF lp SKIP LOCKED

services/libs/data-access-layer/src/mailinglist/index.ts:84

  • Reclaiming a soft-deleted source under another integration preserves its old listProcessing row because this conflict path does nothing. The ownership check intentionally permits deleted rows, while the upsert changes integrationId; consequently the new project starts from the previous project's heads and never receives historical messages. Reset state, timestamps, and lastProcessedHeads when ownership changes.
    backend/src/api/integration/helpers/mailingListAuthenticate.ts:29
  • Restricting the scheme to HTTPS does not prevent SSRF: a tenant editor can still target private/link-local hosts with TLS, and redirects may reach a different host. The worker then performs the server-side fetch. Apply the repository's network URL validation or enforce an explicit public-inbox host allowlist, including redirect validation.
const isSafeSourceUrl = (sourceUrl: string): boolean => {
  try {
    return new URL(sourceUrl).protocol === 'https:'
  } catch {
    return false
  }

backend/src/api/integration/helpers/mailingListAuthenticate.ts:41

  • sourceUrl is persisted and compared byte-for-byte, but the worker strips trailing slashes before cloning. Equivalent inputs such as https://lore.kernel.org/list and https://lore.kernel.org/list/ therefore bypass both duplicate validation and cross-integration ownership checks, creating duplicate ingestion of the same archive. Canonicalize URLs before they reach the service/DAL and enforce uniqueness on that canonical value.
        sourceUrl: z.string().trim().min(1).refine(isSafeSourceUrl, {
          message: 'sourceUrl must be a valid https:// URL',
        }),

Comment on lines +186 to +190
sql_query = """
UPDATE mailinglist."listProcessing"
SET "lastProcessedHeads" = "lastProcessedHeads" || $1::jsonb,
"updatedAt" = NOW()
WHERE "listId" = $2
Comment on lines +1479 to +1480
const currentSegmentId = (options || this.options).currentSegments[0].id
await upsertMailingLists(qx, currentSegmentId, integration.id, lists)
"channel": channel,
"title": subject,
"body": json_body,
"url": source.rstrip("/") + "/r/" + msgid,
GIT = 'git',
CRUNCHBASE = 'crunchbase',
GROUPSIO = 'groupsio',
MAILINGLIST = 'mailinglist',
…318)

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings July 22, 2026 10:54
Signed-off-by: Uroš Marolt <uros@marolt.me>
…1318)

Signed-off-by: Uroš Marolt <uros@marolt.me>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (8)

backend/src/api/integration/helpers/mailingListAuthenticate.ts:26

  • Checking only the scheme still permits authenticated callers to make the worker clone attacker-chosen internal HTTPS hosts (including loopback/private DNS targets), creating a blind SSRF from the worker network. Given the initial hardcoded deployment, allowlisting approved public-inbox hosts is safest; otherwise resolve and reject private/link-local/loopback addresses in the worker and re-check every redirect before fetching.
const isSafeSourceUrl = (sourceUrl: string): boolean => {
  try {
    return new URL(sourceUrl).protocol === 'https:'

services/libs/data-access-layer/src/mailinglist/index.ts:75

  • The preflight ownership query does not make this upsert atomic. Two projects connecting the same previously absent source concurrently can both observe no conflict; the transaction that loses the insert race then executes this unconditional update and silently reassigns the first project's list. Make the conflict update conditional on the existing owner being this integration (or the row being deleted), and surface a conflict when no row is returned.
    services/libs/data-access-layer/src/mailinglist/index.ts:84
  • When a soft-deleted source is connected by a different integration or segment, the list row is re-pointed but this leaves its old processing row and lastProcessedHeads intact. The new project will therefore skip the entire archive up to the previous owner's checkpoint and only receive future messages. Reset processing state when ownership/segment changes, while preserving progress for ordinary updates by the same owner.
    services/apps/mailing_list_integration/src/crowdmail/database/crud.py:86
  • This count check is not serialized with acquiring a row. With multiple worker replicas, several transactions can read the same count below the limit, lock different pending rows, and all proceed, exceeding MAX_CONCURRENT_ONBOARDINGS—the only scenario where a limit above one matters because each process handles one list at a time. Use a transactional semaphore/advisory lock or another atomic capacity mechanism.
            AND l."deletedAt" IS NULL
            AND c.count < $3
        ORDER BY lp.priority ASC, lp."createdAt" ASC
        LIMIT 1

services/apps/mailing_list_integration/src/crowdmail/database/crud.py:190

  • Stale-reclaim can deliberately let two workers process one list, but this checkpoint is not tied to the lease acquired by the caller. A reclaimed worker can later overwrite the same shard with an older SHA; its subsequent mark/release can also overwrite the new worker's state and clear its lock. Carry an acquisition token (for example the acquired lockedAt value or a lease UUID) and require it in every checkpoint, mark, and release WHERE clause.
    if not mailing_list:
        mailing_list = await acquire_recurrent_list()
    return mailing_list

services/apps/mailing_list_integration/src/crowdmail/services/parse/noteren.py:423

  • For a range containing more than one commit, each call prints a separate indented JSON document back-to-back, so the advertised output is neither valid JSON nor valid JSON Lines. Emit one JSON array for the range, or emit one compact JSON object per line and document the output as JSONL.
    services/apps/mailing_list_integration/src/crowdmail/worker/list_worker.py:159
  • The tests added in this service exercise only the parser/CLI; the core flush sequence has no coverage. Please add async worker tests for DB-insert failure, Kafka failure, checkpoint failure, and skipped-message behavior to verify that retries neither lose messages nor advance heads prematurely. This state machine is the reliability boundary of the integration.
    services/apps/mailing_list_integration/src/crowdmail/server.py:13
  • The PR description says the server import and FastAPI lifespan are tested, but the only test module under this service is src/test/test_noteren.py, which never imports crowdmail.server or exercises this lifespan. Add the claimed lifecycle test (including worker startup and timeout/cancellation shutdown), or correct the test-coverage statement.

Comment on lines +101 to +103
heads[shard] = git_id
dirty_heads[shard] = git_id
try:
Comment on lines +1826 to +1830
if (ownerId && !dbMember) {
this.log.warn(
{ memberId: ownerId, identity: conflictIdentity },
'Verified identity already belongs to an existing member — attaching to it instead of creating a new one',
)
Copilot AI review requested due to automatic review settings July 22, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 52 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

backend/src/api/integration/helpers/mailingListAuthenticate.ts:26

  • This validation still permits SSRF targets such as https://127.0.0.1, private RFC1918 addresses, link-local metadata endpoints, and hostnames that resolve internally. The worker later passes this tenant-controlled URL to public-inbox-clone from the service network, so HTTPS alone does not make the fetch safe. Restrict sources to an approved public-inbox host set, or enforce DNS/IP checks (including redirects and DNS rebinding) in the worker before fetching.
const isSafeSourceUrl = (sourceUrl: string): boolean => {
  try {
    return new URL(sourceUrl).protocol === 'https:'

services/libs/data-access-layer/src/mailinglist/index.ts:76

  • The ownership precheck is not atomic with this upsert. Two integrations can both observe no conflict, then the second ON CONFLICT branch silently overwrites the first integration's segmentId/integrationId, defeating the stated ownership protection. Make ownership enforcement part of the write (for example, lock the source row or use a conditional conflict update) and surface a conflict when the atomic write does not acquire ownership.
    services/apps/mailing_list_integration/src/crowdmail/database/crud.py:211
  • Merging JSON does not prevent a stale worker from moving the same shard backward. A live run can exceed the 4/12-hour reclaim timeout, allowing a second worker to start from its checkpoint; either worker can then overwrite that shard key, and stale runs can also mark/release state owned by the newer run. Add a renewable lease/token and make checkpoints, completion, and release conditional on still owning that lease (or otherwise compare shard progress monotonically).
        SET "lastProcessedHeads" = "lastProcessedHeads" || $1::jsonb,

Comment on lines +1759 to +1762
const IDENTITY_CONSTRAINTS = new Set([
'uix_memberIdentities_platform_value_type_verified',
'uix_memberIdentities_memberId_platform_value_type',
])
Copilot AI review requested due to automatic review settings July 22, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants